fix(evpn-bridge): fix for frr to handle vrf setup and teardown #401
Conversation
pkg/frr/frr.go
Outdated
| if err != nil { | ||
| data, err = Frr.FrrZebraCmd(ctx, fmt.Sprintf("configure terminal\n %s\n exit\n", delCmd2)) | ||
| if err != nil || checkFrrResult(data, false) { | ||
| log.Printf("FRR: Error %s\n", data) |
There was a problem hiding this comment.
Does data contain error description?
There was a problem hiding this comment.
Yes data contains the complete FRR command output.
There was a problem hiding this comment.
would it be better in case of an error, in FrrBgpCmd, just add data content into err and return empty first argument?
pkg/frr/frr.go
Outdated
| @@ -437,7 +430,7 @@ func setUpVrf(vrf *infradb.Vrf) (string, bool) { | |||
|
|
|||
| // checkFrrResult checks the vrf result | |||
| func checkFrrResult(cp string, show bool) bool { | |||
| return ((show && reflect.ValueOf(cp).IsZero()) || strings.Contains(cp, "warning") || strings.Contains(cp, "unknown") || strings.Contains(cp, "Unknown") || strings.Contains(cp, "Warning") || strings.Contains(cp, "Ambiguous") || strings.Contains(cp, "specified does not exist")) | |||
| return ((show && reflect.ValueOf(cp).IsZero()) || strings.Contains(cp, "warning") || strings.Contains(cp, "unknown") || strings.Contains(cp, "Unknown") || strings.Contains(cp, "Warning") || strings.Contains(cp, "Ambiguous") || strings.Contains(cp, "specified does not exist") || strings.Contains(cp, "Error")) | |||
There was a problem hiding this comment.
reflect.ValueOf(cp).IsZero()) -> cp == ""
what if cp contains waRning? Will you add a new comparison then? Will it be better to lower case the string and only then check what itcontains?
There was a problem hiding this comment.
- all local vars should start from lowercase. I see
Patternsin that function. Pls check the code you touched in the PR - do not use
reflect.ValueOf(...).IsZero()frequently, you can use simple comparison. Pls check all cases
There was a problem hiding this comment.
Changed all local vars names in frr.go and replaced all reflect.valueof IsZero with simple comparison.
pkg/frr/frr.go
Outdated
| } | ||
| hname, _ := os.Hostname() | ||
| L2vpnCmd := strings.Split(cp, "json") | ||
| L2vpnCmd = strings.Split(L2vpnCmd[1], hname) | ||
| cp = L2vpnCmd[0] | ||
| // fmt.Printf("FRR_L2vpn[0]: %s\n",cp) | ||
| if len(cp) != 7 { | ||
| if len(cp) != 7 { // Checking CMD o/p |
There was a problem hiding this comment.
magic number. Rely on string length is very unreliable
There was a problem hiding this comment.
Addressed this and changed to string compare instead of string length
There was a problem hiding this comment.
is response should be in json format? Should we better try to Unmarshall it then using"\r\n{}\r\n{\r\n}\r\n"? If not, maybe we could check if it contains alphanumerical values instead?
There was a problem hiding this comment.
Changed the comparing the string with alphanumerical.
atulpatel261194
force-pushed
the
opi_frr_bug_main_pr
branch
from
585cd47
to
138318f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
===========================================
- Coverage 50.77% 20.23% -30.54%
===========================================
Files 37 51 +14
Lines 2525 6863 +4338
===========================================
+ Hits 1282 1389 +107
- Misses 1114 5306 +4192
- Partials 129 168 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
venkyvsp
force-pushed
the
opi_frr_bug_main_pr
branch
2 times, most recently
from
a553e28
to
5ab2fc8
Compare
atulpatel261194
force-pushed
the
opi_frr_bug_main_pr
branch
from
05da753
to
a8da21f
Compare
Signed-off-by: Venkatesh, Vemula <venkatesh.vemula@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
Signed-off-by: atulpatel261194 <Atul.Patel@intel.com>
atulpatel261194
force-pushed
the
opi_frr_bug_main_pr
branch
from
4ee351a
to
165664c
Compare
fix: frr handle the duplicate vni and deleting of non-existence vrf and svi.